-
Notifications
You must be signed in to change notification settings - Fork 303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Monorepo structure #2300
base: master
Are you sure you want to change the base?
Monorepo structure #2300
Conversation
9431214
to
e23cfef
Compare
@Desplandis I resolve some issues
|
@gchoqueux Nice! I will take a quick look of your changes and write a todo list of things that should absolutely be tested before reviewing more thoroughly this PR. Expect it for tomorrow or next tuesday! @jailln @mgermerie @ftoromanoff @AnthonyGlt I think we'll need your inputs on this change! ;) This should be a good start to fix #2197, #1930, #2201 (list not exhaustive, feel free to complete). Shall we create a meta-issue to aggregate all those issues? |
e5ec2ea
to
b2308ff
Compare
ea0116d
to
962c99d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! This is first batch of my review of this PR. Unfortunately I'm kinda blocked on testing the distribution since the compiled code for the itowns
package is incorrect (especially with imports to relative modules).
Moreover, there is a lot of points that I would like you to address. I'll resume the review once those points have been resolved.
Since this PR is massive, @jailln there may be some things that eluded me, do not hesitate to complement this review. =)
"test-with-coverage_lcov": "c8 -n src --reporter=lcov cross-env npm run test-unit", | ||
"watch": "cross-env BABEL_DISABLE_CACHE=1 babel --watch src --out-dir lib", | ||
"postinstall": "cross-env NO_UPDATE_NOTIFIER=true node ./scripts/prepare.mjs && node ./scripts/replace.config.mjs", | ||
"prepublishOnly": "npx copyfiles -u 1 \"../../examples/**/*\" ./examples/ && npx copyfiles -u 1 \"../../docs/**/*\" ./docs/ && npx copyfiles -u 1 \"../../dist/**/*\" ./dist/ ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just not distribute examples and docs like most other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be decided
d578888
to
1bd17ac
Compare
f0ddd49
to
61e950f
Compare
I've finished answering/fixing all your comments. |
61e950f
to
af45df1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not much to add to @Desplandis review.
I just think we should explain this new structure in the README and add sub package installation possibilities.
Also, how does it work with releasing and versioning? does subpackage all have the same version or can we (should we?) release them independently ?
ac0a1ad
to
451da03
Compare
Yes, I'll document
In a monorepo, it's more complicated to manage a different version for each module, but if we can find a simple and robust solution we can implement it later. |
6f3ada2
to
d4616ba
Compare
d4616ba
to
b1d0ba1
Compare
@gchoqueux Did you try to publish it in your own This is the a log of my last attempt: The CI works correctly but I would like to ensure that both npm and website publishing are working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last batch of comments for me, only documentation remarks, I let you see with @Desplandis for package publishing. Thanks for the changes you already made
The geographic package provides utilities for handling coordinates, ellipsoids, extents and rotations across different coordinate systems. | ||
|
||
* [Coordinates](http://www.itowns-project.org/itowns/docs/#api/Geographic/Coordinates) : A Coordinates object (geodetic datum), defined by a [crs] and three values. | ||
* [Extent](http://www.itowns-project.org/itowns/docs/#api/Geographic/Extent) : Extent is geographical bounding rectangle defined by 4 limits: west, east, south and north. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add: and a crs
?
|
||
The geographic package provides utilities for handling coordinates, ellipsoids, extents and rotations across different coordinate systems. | ||
|
||
* [Coordinates](http://www.itowns-project.org/itowns/docs/#api/Geographic/Coordinates) : A Coordinates object (geodetic datum), defined by a [crs] and three values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you replace [crs]
by Coordinate Reference System (crs)
|
||
The geographic package provides utilities for handling coordinates, ellipsoids, extents and rotations across different coordinate systems. | ||
|
||
* [Coordinates](http://www.itowns-project.org/itowns/docs/#api/Geographic/Coordinates) : A Coordinates object (geodetic datum), defined by a [crs] and three values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you organize this list alphabetically please? and add missing classes and variables (CoordStars and ellipsoidSizes). BTW I would for renaming ellipsoidSizes to WGS84EllipsoidSize while we're at it.
|
||
* [Coordinates](http://www.itowns-project.org/itowns/docs/#api/Geographic/Coordinates) : A Coordinates object (geodetic datum), defined by a [crs] and three values. | ||
* [Extent](http://www.itowns-project.org/itowns/docs/#api/Geographic/Extent) : Extent is geographical bounding rectangle defined by 4 limits: west, east, south and north. | ||
* [Crs](http://www.itowns-project.org/itowns/docs/#api/Geographic/CRS) : This module provides basic methods to manipulate a CRS (as a string). Visiting [epsg.io](https://epsg.io/) to more coordinate system worldwide. Use **Proj4js** definition in epsg.io. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visiting [epsg.io](https://epsg.io/) to more coordinate system worldwide. Use **Proj4js** definition in epsg.io.
-> Visit epsg.io to explore available coordinates systems and use their proj4js definition to define them in itowns with proj4.defs(crs, proj4def)
* [Coordinates](http://www.itowns-project.org/itowns/docs/#api/Geographic/Coordinates) : A Coordinates object (geodetic datum), defined by a [crs] and three values. | ||
* [Extent](http://www.itowns-project.org/itowns/docs/#api/Geographic/Extent) : Extent is geographical bounding rectangle defined by 4 limits: west, east, south and north. | ||
* [Crs](http://www.itowns-project.org/itowns/docs/#api/Geographic/CRS) : This module provides basic methods to manipulate a CRS (as a string). Visiting [epsg.io](https://epsg.io/) to more coordinate system worldwide. Use **Proj4js** definition in epsg.io. | ||
* [OrientationUtils](http://www.itowns-project.org/itowns/docs/#api/Geographic/OrientationUtils) : it provides methods to compute the quaternion that models a rotation defined with various conventions, including between different CRS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utilities to compute a rotation quaternion from various rotation conventions, including between different crs.
* [Extent](http://www.itowns-project.org/itowns/docs/#api/Geographic/Extent) : Extent is geographical bounding rectangle defined by 4 limits: west, east, south and north. | ||
* [Crs](http://www.itowns-project.org/itowns/docs/#api/Geographic/CRS) : This module provides basic methods to manipulate a CRS (as a string). Visiting [epsg.io](https://epsg.io/) to more coordinate system worldwide. Use **Proj4js** definition in epsg.io. | ||
* [OrientationUtils](http://www.itowns-project.org/itowns/docs/#api/Geographic/OrientationUtils) : it provides methods to compute the quaternion that models a rotation defined with various conventions, including between different CRS. | ||
* Ellipsoid : In geodesy, a reference ellipsoid is a mathematically defined surface that approximates the geoid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proposition: A representation of an [ellipsoid](https://en.wikipedia.org/wiki/Ellipsoid) and useful computation methods (geodetic normal, etc.)
|
||
// change projection system to EPSG:2154 | ||
|
||
// defs EPSG:2154 crs (visiting epsg.io to get new definition and export to Proj4js) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- visiting -> visit
- to get new definition and export to Proj4js : to get the proj4js definition of a crs
// to the geocentric frame (epsg:4978) | ||
quat_crs2crs = OrientationUtils.quaternionFromCRSToCRS("EPSG:2154", "EPSG:4978")(origin); | ||
// Compute the rotation of a sensor platform defined by its attitude | ||
quat_attitude = OrientationUtils.quaternionFromAttitude(attitude); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for these examples :) can you define a mock attitude above to ease understanding for newbies please?
```bash | ||
npm install --save itowns | ||
``` | ||
|
||
```js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to replace by something more generic and that will be easier to update when new packages are added and to remove examples since we already have examples in each sub-package documentation:
iTowns is currently moving to a monorepo organization and to a segmentation in sub-modules, allowing to import only some of itowns functionalities. Current itowns sub-modules are:
- [@itowns/geographic](packages/Geographic/README.md): `npm install --save @itowns/geographic`
- [@itowns/widgets](packages/Widgets/README.md): `npm install --save @itowns/widgets`
- [@itowns/core](packages/Main/README.md); `npm install --save @itowns/core`
Note the s at the end of @itowns/widgets
since there is more: can you update this package name and also the folder name: packages/Widget -> packages/Widgets
@@ -0,0 +1,131 @@ | |||
![iTowns](https://raw.githubusercontent.com/iTowns/itowns.github.io/master/images/itowns_logo_300x134.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not copy a part of the parent README here but just explains that is contains all other itowns functionalities and link to the parent README of the repo and to the doc instead.
Description
Refactoring iTowns to monorepo structure.
The goal is to split itowns down into feature packages.
As an example, I've started with the geographic functionalities. (@itowns/geographic)
exportsFields
For the moment
Debug
andWidget
modules are private.Motivation and Context
Split code in packages for clearly structured code.
Simplifies development and facilitates contributions.
Increases the scope of users who only want to use a few functions.
This structure makes it necessary to make functions independent
Code movement
The classes
Coordinates, Crs, Ellipsoid, Extent, OrientationUtils
files are moved to./packages/geographic/src
.The
proj4
dependency moves to geographic package.The unit tests moves to
packages/xxx/test
.the code
utils/debug
movespackages/Debug
the code
src/Utils/Gui
movespackages/Widget
dependencies
This proposal from monorepo doesn't use monorepo package (by example Lerna).
New dev dependency is
Concurrently
, it's used to watch script in parallelItowns dependencies
(three, proj4...) are moved to the packages that use themnode scripts
The scripts uses the options
--workspaces
or-ws
, to call the corresponding script in each sub packages.